Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

webhook: Disable HTTP2 by default #522

Merged
merged 1 commit into from
Oct 24, 2023
Merged

webhook: Disable HTTP2 by default #522

merged 1 commit into from
Oct 24, 2023

Conversation

cgoncalves
Copy link
Contributor

To address CVE-2023-44487.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@cgoncalves cgoncalves changed the title Update golang.org/x/net Update sigs.k8s.io/controller-runtime to 0.16.3 Oct 18, 2023
@adrianchiris
Copy link
Collaborator

seems some apis from controller-runtime have changed as build is failling

@zeeke @cgoncalves
webhook: Disable HTTP2 by default
2eefedb 
From docs: net/http/server.go

"[...] If TLSNextProto is not nil, HTTP/2 support
is not enabled automatically."
Server.TLSNextProto

Signed-off-by: Andrea Panattoni <[email protected]>
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@cgoncalves cgoncalves changed the title Update sigs.k8s.io/controller-runtime to 0.16.3 webhook: Disable HTTP2 by default Oct 20, 2023
@cgoncalves
Copy link
Contributor Author

controller-runtime v0.15.3 will include the x/net bump to 0.17.0 but it hasn't been released yet.
controller-runtime v0.16.3 breaks APIs (around metrics and namespaces, easily fixable) but may not be desired at this point in time.
Andrea suggested disabling HTTP/2 on the webhook server. Other projects are doing so as well for the time being.

SchSeba
SchSeba reviewed Oct 20, 2023
View reviewed changes
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

the only comment I have is we don't have a way to change this variable so it will always by disable.

maybe we can just disable it for now until the controller-runtime gets updated?

@cgoncalves cgoncalves mentioned this pull request Oct 20, 2023
Merged
zeeke
zeeke approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only comment I have is we don't have a way to change this variable so it will always by disable.

maybe we can just disable it for now until the controller-runtime gets updated?

u/s user can enable http2 back by editing the deployment files.

LGTM

SchSeba
SchSeba approved these changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zeeke
Copy link
Member

zeeke commented Oct 24, 2023

@e0ne , @adrianchiris , @Eoghan1232 Please, take a look. we need this to fix a CVE

Eoghan1232
Eoghan1232 approved these changes Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@Eoghan1232 Eoghan1232 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Eoghan1232 Eoghan1232 merged commit de7945c into k8snetworkplumbingwg:master Oct 24, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eoghan1232 Eoghan1232 Eoghan1232 approved these changes

@SchSeba SchSeba SchSeba approved these changes

@zeeke zeeke zeeke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cgoncalves @adrianchiris @zeeke @SchSeba @Eoghan1232